-
Notifications
You must be signed in to change notification settings - Fork 16
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: support arbitrary field dependents #340
base: main
Are you sure you want to change the base?
Conversation
tests are failing during building of mypyc, due to pydantic/pydantic#10907 |
CodSpeed Performance ReportMerging #340 will not alter performanceComparing Summary
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #340 +/- ##
===========================================
- Coverage 100.00% 99.95% -0.05%
===========================================
Files 21 21
Lines 2095 2105 +10
===========================================
+ Hits 2095 2104 +9
- Misses 0 1 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can we not use the same machinery like current property setter/getter (put things in self._changes_queue
)?
name: pydantic.fields.FieldInfo(annotation=f.return_type, frozen=False) | ||
for name, f in cls.model_computed_fields.items() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not see test with pydantic 1.x, Should be here: if hasattr(cls, "model_computed_fields")
?
# An attribute is changing that is not in the SignalGroup | ||
# if it has field dependents, we must still continue | ||
# to check the _changes_queue | ||
must_continue = name in self.__field_dependents__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
must_continue = name in self.__field_dependents__ | |
must_continue = must_continue or name in self.__field_dependents__ |
As it may happen in the middle of for loop
# primary changes should contains only fields | ||
# that are changed directly by assignment |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not fully understand. When we need to emit a signal. This comment still statement that self._primary_changes
are only fields changed by direct assignment.
And documentation of https://docs.pydantic.dev/2.0/usage/computed_fields/ do not mention the assigment of this property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i wasn't entirely clear on the _primary_changes
, and _changes_queue
attributes here. So maybe I did it wrong. we need to emit a signal here when a non-field attribute changes, provided that some other evented field lists that attribute as one of its "dependents". So, in the example provided by the original requester:
from psygnal import EventedModel
from pydantic import computed_field, PrivateAttr
class MyModel(EventedModel):
_items_dict: dict[str, int] = PrivateAttr(default_factory=dict)
@computed_field
def item_names(self) -> list[int]:
return list(self._items_dict.keys())
def add_item(self, name: str, value: int) -> None:
# this next assignment should trigger the emission of `self.events.item_names`
# because `_items_dict` was listed as a field_dependency of `item_names`
self._items_dict = {**self._items_dict, name: value}
class Config:
field_dependencies = {"item_names": ["_items_dict"]}
if I did that incorrectly (or stupidly) with the existing _primary_changes/_changes_queue
pattern let me know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think this might be the first case where the "primary change" is to a non-evented attribute (_items_dict
)... and so it didn't quite fit into the existing pattern. It's the first case where a _primary_change
isn't actually able to itself be evented
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm.
Maybe we should add a dummy emiter (callback that do nothing, and raises an exception on connection) for private attributes that are listed in field_dependencies
? Then we will move the complexity from setattr to constructor call?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's worth trying, lemme see how it goes
This is a proof of principle PR that would support the feature that @sjdemartini is requesting in #337. namely:
@Czaki, I would appreciate your eyes on this.
_check_if_values_changed_and_emit_if_needed
area that you had previously worked on)test_private_field_dependents
closes #337